add support for ECC_MAKE_PUB and ECC_CHECK_PUB crypto callbacks#451
add support for ECC_MAKE_PUB and ECC_CHECK_PUB crypto callbacks#451rizlik wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends wolfHSM’s ECC public-key handling by adding end-to-end support (wire messages, client APIs, crypto-callback plumbing, server handlers, and tests) for two wolfCrypt callback operations: deriving a public point from a private-only ECC key (WC_PK_TYPE_EC_MAKE_PUB) and validating ECC keys (WC_PK_TYPE_EC_CHECK_PUB_KEY). It also updates server-side public export to handle resident private-only ECC keys by deriving their public half before encoding.
Changes:
- Add new crypto wire messages + translation helpers for ECC make-public requests/responses, and extend ECC check request fields.
- Implement client API + crypto callback routing and server request handlers for ECC make-public and check-pubkey operations.
- Expand test coverage to exercise make-public and check-pubkey paths (including private-only and mismatch scenarios) and update feature documentation.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfhsm/wh_settings.h | Adds a client-build compile-time guard against WOLFSSL_VALIDATE_ECC_IMPORT due to unsafe nested transaction/buffer reuse. |
| wolfhsm/wh_message_crypto.h | Introduces ECC make-public request/response structs and extends ECC check request fields. |
| wolfhsm/wh_client_crypto.h | Adds/updates public client APIs for ECC make-public and ECC pubkey validation with detailed contract docs. |
| src/wh_message_crypto.c | Implements translation helpers for new ECC make-public messages and additional ECC check request fields. |
| src/wh_client_cryptocb.c | Routes new wolfCrypt crypto-callback ops (EC_MAKE_PUB, EC_CHECK_PUB_KEY) to wolfHSM client APIs. |
| src/wh_client_crypto.c | Implements request/response flows for wh_Client_EccMakePub and expanded wh_Client_EccCheckPubKey. |
| src/wh_server_crypto.c | Adds server-side handlers for EC_MAKE_PUB and EC_CHECK_PUB_KEY and dispatches them in crypto request handling. |
| src/wh_server_keystore.c | Uses server devId for key init in export helpers and derives ECC public half for resident private-only keys before DER export. |
| test/wh_test_crypto.c | Adds extensive ECC make-public and check-pubkey functional tests; expands DMA public export coverage for private-only ECC keys. |
| test/wh_test_check_struct_padding.c | Includes new ECC make-public structs in padding checks. |
| docs/src/5-Features.md | Updates feature list to reflect ECC public key derivation and key validation support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (WH_KEYID_ISERASED(key_id)) { | ||
| /* Must import the key to the server and evict it afterwards */ | ||
| uint8_t keyLabel[] = "TempEccMakePub"; | ||
| whNvmFlags flags = WH_NVM_FLAGS_USAGE_ANY; |
There was a problem hiding this comment.
we should probably restrict usage flags to only what is necessary here. Also do you think we should set ephemeral flag?
| if (WH_KEYID_ISERASED(key_id)) { | ||
| /* Must import the key to the server and evict it afterwards */ | ||
| uint8_t keyLabel[] = "TempEccCheck"; | ||
| whNvmFlags flags = WH_NVM_FLAGS_USAGE_ANY; |
There was a problem hiding this comment.
same comment on flags as above
| int check_order = info->pk.ecc_check_pub.checkOrder; | ||
| int check_priv = info->pk.ecc_check_pub.checkPriv; | ||
|
|
||
| if (pub_key_len > 0xFFFFU) { |
There was a problem hiding this comment.
consider using UINTx_MAX here
| if (out_pub_len != NULL) { | ||
| /* Clamp rather than truncate: an oversized capacity must not | ||
| * wrap to a small one and provoke a spurious BUFFER_E. */ | ||
| pub_len = (*out_pub_len > 0xFFFFU) ? 0xFFFFU |
There was a problem hiding this comment.
consider using UINTx_MAX here
| (void)req.curveId; | ||
| (void)req.checkOrder; | ||
| (void)req.checkPriv; |
There was a problem hiding this comment.
should we just remove these if they are unused then?
|
@rizlik just a few nits+questions, overall looks great. |
No description provided.